Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dropdown Items in Navbar #13

Merged
merged 23 commits into from
Apr 16, 2018
Merged

Dropdown Items in Navbar #13

merged 23 commits into from
Apr 16, 2018

Conversation

ulivz
Copy link
Member

@ulivz ulivz commented Apr 14, 2018

Summary

This branch is for the dropdown Items in navbar, and fully leverage the style from vue offical website.

Changes

  • Support dropdown items in navbar.
  • Support nested dropdown item.
  • Fixed a bug of using external link(External Navbar Links #37) at a nested route which will take user to somewhere like **guide/https://vuejs.org/**.

Prewview

@@ -48,6 +48,25 @@ module.exports = {
}
```

The item in `themeConfig.nav` could also be a dropdown menu:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those links can also be dropdown menus, add nested items via items:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed at 50689ea, thanks

<router-link
class="router-link"
:to="item.link"
v-if="!isExternal(item.link)">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better put the > on the next line, stuck to {{ item.text }} and stick the next router-link to the end of it too. But I see it comes this way from the original code 🤔

Copy link
Member Author

@ulivz ulivz Apr 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, when I checked existing code, I found that the original code is always this way:

<!-- lib/default-theme/SidebarGroup.vue -->
 <span class="arrow"
        v-if="collapsable"
        :class="open ? 'up' : 'down'"></span>

And Is that what you say?

  <router-link
    class="router-link"
    :to="item.link"
    v-if="!isExternal(item.link)"
  >{{ item.text }}</router-link>

But I cannot find this style at existing code. or these two following styles would be better? it's most likely to original style.

  <router-link class="router-link"
    :to="item.link"
    v-if="!isExternal(item.link)">
    {{ item.text }}
  </router-link>

or

  <router-link class="router-link"
    :to="item.link"
    v-if="!isExternal(item.link)">{{ item.text }}</router-link>

Can you give a code snippet that you think is reasonable? Thank you.

Copy link
Member

@posva posva Apr 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's pretty much what I said

The reasonable one would be:

  <router-link
    class="router-link"
    :to="item.link"
    v-if="!isExternal(item.link)"
  >{{ item.text }}</router-link>

which doesn't create any extra white space around the text, but, let's wait for Evan input on this one

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, fixed at 7eb2563

v-else
:href="item.link"
target="_blank"
class="router-link">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

required: true
}
},
mixins: [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you use a mixin here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, using a mixin here is to reuse the existing method. or you would have a better way to do it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can just put the methods object without the mixins, it would also work

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, long time to write Vue, I just forgot some important thing, thanks.

return (this.$site.themeConfig.nav || []).map(({ text, link, type, items }) => ({
text,
type,
link: link ? ensureExt(link) : void 0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe something more readable like null instead of void 0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed at 50689ea, thanks

top calc(50% - 3px)
left -10px
&:hover
margin-bottom 0
Copy link
Contributor

@egoist egoist Apr 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be moved to a

Copy link
Member Author

@ulivz ulivz Apr 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AWESOME! fixed at 6de2efa, thank for finding it out.

&:after
content: ""
width: 0
height: 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no :

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed at 50689ea, thanks

@ulivz
Copy link
Member Author

ulivz commented Apr 14, 2018

@posva and lovely @egoist, I just addressed the comments that I can fix now, and there are still some pending comments, would you please help me to sync with it? thanks

@ulivz
Copy link
Member Author

ulivz commented Apr 14, 2018

I just checked at vue official doc, and found out this dropdown style, @yyx990803 Do you think I need to finish this feature in this PR? (In vuejs.org, it's hard html code now.)

image

To do that, I just come up with some config styles for that:

  • Flattern style
module.exports = {
  themeConfig: {
    nav: [
      {
        text: 'Ecosystem',
        type: 'dropdown',
        items: [
          { text: 'Help', type: 'label' },   // Or allow to omit the type?
          { text: 'Forum', link: '...' },
          { text: 'Chat', link: '...' },
          { text: 'Tooling' },
          { text: 'Devtools', link: '...' }
          // ...
        ]
      }
    ]
  }
}
  • Flattern style from docute:
module.exports = {
  themeConfig: {
    nav: [
      {
        title: 'Ecosystem',
        type: 'dropdown',
        items: [
          { type: 'label', title: 'Help' },
          // ... items
          { type: 'sep' } // separator
          // ... other items
        ]
      }
    ]
  }
}
  • Semantic style
module.exports = {
  themeConfig: {
    nav: [
      {
        text: 'Ecosystem',
        type: 'dropdown',
        items: [
          {
            text: 'Help',
            items: [
              { text: 'Forum', link: '...' },
              { text: 'Chat', link: '...' }
            ]
          },
          {
            text: 'Tooling',
            items: [
              { text: 'Devtools', link: '...' },
              // ...
            ]
          },
          // ...
        ]
      }
    ]
  }
}

Which do you think is better? or better suggestion?

BTW, at present, when a item does not provide link but provides items, it can be treated as a dropdown, so can we omit passing type=dropdown?

@ulivz
Copy link
Member Author

ulivz commented Apr 14, 2018

@posva Addressed all comments, thanks.

@yyx990803
Copy link
Member

I like style 3 (Semantic). We can treat items as indication of a dropdown.

@ulivz
Copy link
Member Author

ulivz commented Apr 14, 2018

@yyx990803 Got it, I will work at here.

In addition, I am also working for the Multi-Language Support based on this dropdown feature.

PR is here. https://github.com/ulivz/vuepress/pull/1, now the basic function is done and the rest of the job is almost translating, welcome to go here and give me some thoughts so that I can make it better. thank you.

@ulivz
Copy link
Member Author

ulivz commented Apr 14, 2018

Updated

Commit: 6c9e1a4

  • Deprecate type: dropdown at add built-in type judgment for link item.
  • Support nested links at dropdown menu and leverage the style from Vue official website.

@ulivz
Copy link
Member Author

ulivz commented Apr 15, 2018

@egoist @posva If there are no other defects, would you please help me to approve, thank you very much!

class="router-link"
:to="link"
v-if="!isExternal(link)"
exact
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 67c758e, should only be exact for the homepage.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed at 96a6cc6.

<li
v-for="childSubItem in subItem.items"
:key="childSubItem.link">
<nav-link :item="childSubItem"></nav-link>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some conflicts due to fixes which are also addressed in this PR. Should be easy to resolve.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All conflict has been resolved.

Copy link
Member

@yyx990803 yyx990803 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Thanks for the great work. Some final small tweaks:

  1. .nav-item should have cursor: pointer

  2. On mobile, the dropdown links should be togglable. Otherwise with a lot of dropdown items, the sidebar of the current page gets pushed down too much.

  3. To implement (2) we should probably extract the dropdown into its own component.

@ulivz
Copy link
Member Author

ulivz commented Apr 16, 2018

@yyx990803 As a fanatic fan of Vue, it's voluntary and my honor to do this.

Thank you for your advices. It looks better now.

@yyx990803
Copy link
Member

Awesome! One final thing: the dropdowns should be collapsed by default on Mobile.

After that we can remove the test dropdown links and ready to merge.

@ulivz
Copy link
Member Author

ulivz commented Apr 16, 2018

Fixed at b3e0583 ~

@yyx990803 yyx990803 merged commit 79f8f14 into vuejs:master Apr 16, 2018
@yyx990803
Copy link
Member

Thanks for the great work! 🎉

meteorlxy pushed a commit that referenced this pull request Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants